feat: add /api/health and /api/health/solvers endpoints#369
feat: add /api/health and /api/health/solvers endpoints#369SameerAliKhan-git wants to merge 2 commits intoEAPD-DRB:mainfrom
Conversation
Adds two GET endpoints for cross-platform diagnostics: - /api/health returns platform info (OS, arch, Python version) and confirms DataStorage is writable - /api/health/solvers reports GLPK and CBC availability using the same three-tier resolution chain as Osemosys._resolve_solver_folder (env var > PATH > bundled) Includes pytest suite with 8 tests covering response shape, field presence, and mocked solver detection scenarios. Tracked under Track: Cross-Platform.
There was a problem hiding this comment.
Pull request overview
Adds diagnostic health-check endpoints to the Flask API to support cross-platform startup verification and solver availability reporting (intended for Electron startup polling and user troubleshooting).
Changes:
- Added
/api/healthendpoint returning platform/runtime and DataStorage writability status. - Added
/api/health/solversendpoint reporting GLPK/CBC discovery status and source. - Registered the new System health blueprint and added pytest coverage for the new endpoints.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
API/Routes/System/HealthRoute.py |
Introduces the new health blueprint and solver detection logic. |
API/app.py |
Registers the new health_api blueprint with the Flask app. |
tests/test_health.py |
Adds pytest tests covering both new health endpoints and mocked solver discovery scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @patch("Routes.System.HealthRoute.shutil.which") | ||
| def test_glpk_found_on_path(self, mock_which, client): | ||
| """When glpsol is on PATH, glpk should report found=True.""" | ||
| def side_effect(name): | ||
| if name in ("glpsol", "glpsol.exe"): | ||
| return "/usr/bin/glpsol" | ||
| return None | ||
| mock_which.side_effect = side_effect | ||
|
|
||
| resp = client.get("/api/health/solvers") | ||
| data = json.loads(resp.data) | ||
| assert data["glpk"]["found"] is True | ||
| assert data["glpk"]["source"] == "path" | ||
|
|
||
| @patch("Routes.System.HealthRoute.shutil.which", return_value=None) | ||
| def test_no_solvers_reports_false(self, mock_which, client): | ||
| """When no solver is found anywhere, anyAvailable should be False.""" | ||
| # also need to make sure bundled dir scan finds nothing | ||
| with patch("Routes.System.HealthRoute.Config") as mock_cfg: | ||
| # point SOLVERs_FOLDER to a temp dir that has nothing | ||
| import tempfile | ||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| mock_cfg.SOLVERs_FOLDER = Path(tmpdir) | ||
| mock_cfg.DATA_STORAGE = Path(tmpdir) | ||
| resp = client.get("/api/health/solvers") | ||
|
|
There was a problem hiding this comment.
These tests patch shutil.which but don't control SOLVER_GLPK_PATH / SOLVER_CBC_PATH. If a developer/CI environment has either env var set, _check_solver will take the env-var branch and the assertions here can fail (or skip the patched PATH behavior). Consider clearing those env vars in the test (e.g., via monkeypatch.delenv) to make the tests deterministic.
API/Routes/System/HealthRoute.py
Outdated
| if env_val: | ||
| env_path = Path(env_val).expanduser() | ||
| if env_path.is_file(): | ||
| return {"found": True, "source": "env", "path": str(env_path)} |
There was a problem hiding this comment.
When SOLVER_*_PATH points to a file, this reports found=True without verifying that the file is actually the expected solver binary name (e.g., glpsol/glpsol.exe). This can produce false positives vs the resolver logic in OsemosysClass._find_solver_binary, which validates the filename. Consider validating the basename against the allowed binary names before returning found=True, or reusing the existing Osemosys helper logic.
| return {"found": True, "source": "env", "path": str(env_path)} | |
| # Only accept the env file if its basename matches an allowed solver binary name | |
| if env_path.name in _binary_names(binary_name): | |
| return {"found": True, "source": "env", "path": str(env_path)} | |
| # Otherwise, treat as not found via env and continue with other resolution steps |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
API/Routes/System/HealthRoute.py
Outdated
| except Exception as e: | ||
| return jsonify({"status": "error", "message": str(e)}), 500 |
There was a problem hiding this comment.
Catching a broad Exception and returning str(e) in the JSON response can leak internal details like filesystem paths or configuration values. Consider logging the exception server-side and returning a generic error message (or a stable error code) to clients instead.
|
|
||
| response = { | ||
| "status": "ok", | ||
| "platform": platform.system(), | ||
| "architecture": platform.machine(), | ||
| "python": platform.python_version(), | ||
| "dataStorage": "writable" if storage_ok else "error" | ||
| } | ||
| return jsonify(response), 200 |
There was a problem hiding this comment.
The response always sets "status": "ok" and returns HTTP 200 even when dataStorage is not writable (dataStorage="error"). For a readiness/liveness endpoint used by startup guards, this makes it hard to distinguish a healthy backend from a degraded one. Consider setting status to "error" (and returning 5xx/503) when storage_ok is false, or splitting liveness vs readiness semantics explicitly.
| response = { | |
| "status": "ok", | |
| "platform": platform.system(), | |
| "architecture": platform.machine(), | |
| "python": platform.python_version(), | |
| "dataStorage": "writable" if storage_ok else "error" | |
| } | |
| return jsonify(response), 200 | |
| overall_status = "ok" if storage_ok else "error" | |
| http_status = 200 if storage_ok else 503 | |
| response = { | |
| "status": overall_status, | |
| "platform": platform.system(), | |
| "architecture": platform.machine(), | |
| "python": platform.python_version(), | |
| "dataStorage": "writable" if storage_ok else "error" | |
| } | |
| return jsonify(response), http_status |
API/Routes/System/HealthRoute.py
Outdated
| except Exception as e: | ||
| return jsonify({"status": "error", "message": str(e)}), 500 |
There was a problem hiding this comment.
Same concern here: returning the raw exception string in the API response can expose internal paths/config. Prefer logging and returning a generic message or error code.
| @patch("Routes.System.HealthRoute.shutil.which") | ||
| def test_glpk_found_on_path(self, mock_which, client): | ||
| """When glpsol is on PATH, glpk should report found=True.""" | ||
| def side_effect(name): | ||
| if name in ("glpsol", "glpsol.exe"): | ||
| return "/usr/bin/glpsol" | ||
| return None | ||
| mock_which.side_effect = side_effect | ||
|
|
||
| resp = client.get("/api/health/solvers") | ||
| data = json.loads(resp.data) | ||
| assert data["glpk"]["found"] is True | ||
| assert data["glpk"]["source"] == "path" | ||
|
|
There was a problem hiding this comment.
This test assumes the solver env vars are unset so that PATH resolution is exercised. If SOLVER_GLPK_PATH is set in the test runner environment, _check_solver will take the env-var branch and the assertion source=="path" can fail. Use monkeypatch/delenv (or patch.dict on os.environ) to explicitly clear SOLVER_GLPK_PATH and SOLVER_CBC_PATH for deterministic results.
| with patch("Routes.System.HealthRoute.Config") as mock_cfg: | ||
| # point SOLVERs_FOLDER to a temp dir that has nothing | ||
| import tempfile | ||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| mock_cfg.SOLVERs_FOLDER = Path(tmpdir) | ||
| mock_cfg.DATA_STORAGE = Path(tmpdir) | ||
| resp = client.get("/api/health/solvers") |
There was a problem hiding this comment.
Similarly, this test can become environment-dependent if SOLVER_GLPK_PATH/SOLVER_CBC_PATH are set in the runner environment, since the env-var branch runs before PATH/bundled checks. Explicitly clear those env vars in the test to make sure the mocked shutil.which + empty bundled dir are what drive the result.
| with patch("Routes.System.HealthRoute.Config") as mock_cfg: | |
| # point SOLVERs_FOLDER to a temp dir that has nothing | |
| import tempfile | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| mock_cfg.SOLVERs_FOLDER = Path(tmpdir) | |
| mock_cfg.DATA_STORAGE = Path(tmpdir) | |
| resp = client.get("/api/health/solvers") | |
| # and that any solver path environment variables do not interfere | |
| orig_glpk = os.environ.pop("SOLVER_GLPK_PATH", None) | |
| orig_cbc = os.environ.pop("SOLVER_CBC_PATH", None) | |
| try: | |
| with patch("Routes.System.HealthRoute.Config") as mock_cfg: | |
| # point SOLVERs_FOLDER to a temp dir that has nothing | |
| import tempfile | |
| with tempfile.TemporaryDirectory() as tmpdir: | |
| mock_cfg.SOLVERs_FOLDER = Path(tmpdir) | |
| mock_cfg.DATA_STORAGE = Path(tmpdir) | |
| resp = client.get("/api/health/solvers") | |
| finally: | |
| if orig_glpk is not None: | |
| os.environ["SOLVER_GLPK_PATH"] = orig_glpk | |
| if orig_cbc is not None: | |
| os.environ["SOLVER_CBC_PATH"] = orig_cbc |
- implemented solver check caching with module variables - optimized bundled binary search by targeting specific subfolders - added binary filename validation for solver environment variables - improved security by avoiding leakage of raw exception strings to clients - updated health check to return 503 Service Unavailable on storage failure - improved test determinism with monkeypatch and cache resetting Ref: EAPD-DRB#367
Linked issue
Existing related work reviewed
Overlap assessment
Why this PR should proceed
Summary
API/Routes/System/HealthRoute.pywith two GET endpoints:/api/health— returns OS, architecture, Python version, DataStorage writability/api/health/solvers— returns GLPK/CBC availability using the same resolution logic asOsemosys._resolve_solver_folderhealth_apiblueprint inAPI/app.pytests/test_health.pywith 8 pytest testsValidation
Test output (Windows 11, Python 3.13):
